Skip to content

Fix GH-8563#8651

Closed
Girgias wants to merge 2 commits into
php:PHP-8.0from
Girgias:fix-gh8563
Closed

Fix GH-8563#8651
Girgias wants to merge 2 commits into
php:PHP-8.0from
Girgias:fix-gh8563

Conversation

@Girgias

@Girgias Girgias commented May 29, 2022

Copy link
Copy Markdown
Member

With memory streams if we get a NULL buffer we must not instantiate an empty line.

Splitting this from #8644 as this one seems to be fixed properly

@Girgias Girgias requested a review from cmb69 May 29, 2022 16:15
@Girgias

Girgias commented May 29, 2022

Copy link
Copy Markdown
Member Author

Ah great, this hits the issue 188b1d4 which was fixed in 8.1

@Girgias

Girgias commented May 29, 2022

Copy link
Copy Markdown
Member Author

Actually... I'm wondering if this is going to break non standard CSV files which have an empty line somewhere in the file (which is obviously untested).

@cmb69

cmb69 commented May 31, 2022

Copy link
Copy Markdown
Member

I'm wondering if this is going to break non standard CSV files which have an empty line somewhere in the file (which is obviously untested).

I think we should add tests for that. The current behavior is somewhat suprising: https://3v4l.org/PS0El; only if SplFileObject::DROP_NEW_LINE is set, empty lines will be skipped. This is consistent with ~SplFileObject::READ_CSV, but still suprising, because the line breaks are stripped from normal CSV reading.

Girgias added 2 commits June 8, 2022 11:50
With memory streams if we get a NULL buffer we must not instantiate an empty line
@Girgias

Girgias commented Jun 8, 2022

Copy link
Copy Markdown
Member Author

Seems like my fix might indeed be correct as it does not break it (however while merging up the NULL will need to be changed to false)

@cmb69 cmb69 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this is good to be merged.

I'm not sure, though, about renaming the existing tests (i.e. moving them to a subdirectory); maybe postpone that to "master".

@Girgias

Girgias commented Jun 20, 2022

Copy link
Copy Markdown
Member Author

Merged as 6f87a5c

@Girgias Girgias closed this Jun 20, 2022
@Girgias Girgias deleted the fix-gh8563 branch June 20, 2022 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants